Skip to content

fix(release): harden version 7 for stable release - #1025

Merged
bokelley merged 11 commits into
mainfrom
ship-version-7-release
Aug 16, 2026
Merged

fix(release): harden version 7 for stable release#1025
bokelley merged 11 commits into
mainfrom
ship-version-7-release

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Tighten the version 7 release candidate and switch Release Please back to stable versioning so the next release PR promotes 7.0.0 instead of producing another RC.

This consolidates the releasable work from PRs #999 and #1000, including their requested review fixes, plus the bounded v7 issues found during the open issue/PR audit.

What changed

  • harden signing resolution, bounded fetches, replay/idempotency atomicity, and PostgreSQL lock-pool isolation
  • supervise timed and canceled decisioning work without letting abandoned sync work escape concurrency limits
  • make MCP outbound request signing work in the writer task and fail closed when signing policy was not prefetched
  • accept the current request-signing JWK purpose for webhook verification while retaining legacy compatibility
  • expose the originating A2A HTTP request to context factories
  • expose A2A push_sender through create_a2a_server, serve, unified transport, and ServeConfig
  • return FORBIDDEN for test-controller attempts against resolved live accounts
  • preserve order while deduplicating schema-defined unique disclosure filters in both request models
  • remove Release Please's RC configuration so v7 can be promoted to stable

Compatibility

Two intentional breaking hardening changes are called out in the commits:

  • PgBackend and lazy PostgreSQL idempotency wiring require a distinct lock_pool
  • decisioning adopters supplying their own executor= must also set timed_sync_get_products_limit=

Both changes fail at construction rather than allowing unsafe runtime behavior.

Validation

  • make pre-push
  • 6,379 passed, 41 skipped, 9 deselected, 1 xfailed
  • 84.55% coverage
  • source and strict adopter mypy checks passed
  • ruff, black, Bandit, generated-code validation, and pre-commit checks passed

Closes #1018
Closes #1017
Closes #1011
Closes #1009
Closes #1008
Closes #971

Supersedes #999 and #1000.

Require PostgreSQL idempotency adopters to provide a distinct lock_pool so advisory-lock waits cannot deadlock the handler query pool.

BREAKING CHANGE: PgBackend and lazy PostgreSQL idempotency wiring now require a distinct lock_pool. Missing or shared lock pools fail at construction.
Keep synchronous admission permits and proposal/idempotency lifecycle ownership until the underlying worker really settles. Async cancellation remains fail-closed because external mutation may already have occurred.

BREAKING CHANGE: adopters that pass executor= to create_adcp_server_from_platform or serve must also set timed_sync_get_products_limit=.
Webhook verification now accepts both the spec-mandated request-signing purpose and the deprecated webhook-signing value while retaining tag-based cross-profile replay protection.

Closes #1018
Derive the operation from the MCP JSON-RPC body and prefetch signing capabilities before enqueueing tools/call, avoiding both ContextVar loss and recursive writer-session deadlock.

Closes #1017
Keep unresolved and misconfigured account paths on PERMISSION_DENIED while matching the mode-gate storyboard for an explicitly resolved live account.

Closes #1011
Comment thread tests/test_decisioning_dispatch.py Fixed
Comment thread tests/test_decisioning_dispatch.py Fixed
Comment thread tests/test_proposal_lifecycle_e2e.py Fixed
Comment thread tests/test_proposal_lifecycle_e2e.py Fixed
@bokelley
bokelley enabled auto-merge (squash) August 16, 2026 16:16

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes on one Major bug. Everything else here is clean, fail-closed hardening — the async JWKS resolver just needs to mirror the sync resolver it diverged from.

Must fix (blocking)

  1. Async CachingJwksResolver.__call__ rejects valid signed requests during every refresh window. src/adcp/signing/jwks.py — the new request_signature_jwks_unavailable raise ("cached JWKS is expired and refresh cooldown has not elapsed") lives in the unlocked pre-check and is never re-evaluated inside async with self._lock. _refresh sets self._last_attempt = now before the ~10s awaited fetch, so at each max_age boundary the first request enters refresh and every concurrent verification arriving during the fetch window computes cache_expired and (now - _last_attempt < cooldown) and rejects — legitimate signed inbound requests get request_signature_jwks_unavailable for the duration of the fetch, recurring every 30 minutes under load. The sync CachingJwksResolver handles this correctly: it raises inside self._refresh_lock after re-reading state, so waiters block and observe the fresh cache. Move the async raise inside the lock and re-evaluate after re-reading _last_successful_refresh/_last_attempt, mirroring the sync path. Fails closed (security-reviewer: no replay window opened), but it's a verification-path availability regression this PR introduces on the default async resolver. code-reviewer: Major.

The sync resolver already does exactly this three lines up; the async one didn't get the memo.

Things I checked

  • Semver signal is correct. The two breaking hardening changes — PgBackend/lazy PG wiring now requiring a distinct lock_pool, and executor= now requiring timed_sync_get_products_limit= — land under fix(signing)! / fix(decisioning)! with BREAKING CHANGE: footers and a Compatibility section in the body. Both fail at construction, not at runtime. push_sender is additive (feat(a2a)); new signing exports (AtomicReplayStore, ReplayClaimResult, supports_atomic_claim) are additive.
  • Sync-executor admission has no permit leak. submit_supervised (time_budget.py) acquires the BoundedSemaphore once and releases exactly once — immediately on executor.submit failure, otherwise via the concurrent.futures done callback through loop.call_soon_threadsafe. Supervised lifecycle tasks are strong-referenced in _SUPERVISED_SYNC_LIFECYCLES / _SUPERVISED_FINALIZATIONS / _SUPERVISED_OPERATIONS so they can't be GC'd mid-flight on the deadline path; the routed-sync ContextVar path acquires its permit inside _run_sync_delegate.
  • Idempotency lock contract holds. dispatch.py (262 net lines) read in full: the hold()/put_if_absent() backend surface is concrete-with-NotImplementedError so existing subclasses stay importable, PgBackend._active_connection reuse is keyed on asyncio.current_task() and only reused in the task that set it, and the legacy process-local fallback warns + deprecates rather than silently degrading. lock_pool is pool fails closed at construction.
  • Replay store is atomic and never evicts a live nonce. claim() is check-and-set under one RLock (in-memory) / pg_advisory_xact_lock + conditional insert in one txn (PG); the indexed min-heap _purge_expired pops only entries with expiry < now and rejects at capacity instead of evicting; caps validated > 0.
  • Bounded fetches close SSRF + decompression-bomb vectors. Redirect handling inside the new client.stream() blocks still rebuilds the IP-pinned transport with follow_redirects=False; _bounded_http counts actual streamed bytes (immune to a missing/lying Content-Length) and rejects non-identity content-encoding before reading.
  • Freshness changes are strict tightenings and match AdCP 3.1.8. JWKS max_age=1800, brand.json split, and revocation dropping _slide_next_update on 304 (a 304 authenticates no new signed next_update) all fail closed. verify_from_agent_url passing resolution.key_origins or {} turns a missing brand-json declaration into a fail-closed reject rather than warn-and-skip. ad-tech-protocol-expert: sound.
  • Unique disclosure filters are the right shape. Both request schemas declare disclosure_positions/disclosure_persistence as uniqueItems: true, minItems: 1; order-preserving dict.fromkeys dedup via WrapValidator (needed to run after enum coercion and short-circuit None past the rebuilt min_length) is lossless, non-breaking, and keeps the SDK from emitting a schema-invalid array. Generator (scripts/generate_ergonomic_coercion.py) and its output (_ergonomic.py) updated in lockstep; no generated_poc/** hand-edits.
  • Webhook accepting request-signing adcp_use is spec-mandated (webhooks reuse the request-signing key; isolation is the tag, not the purpose — expected_tag=WEBHOOK_TAG retained). Closes #1018.
  • Test-controller FORBIDDEN closes a real live-account bypass (env_sandbox could previously flip allowed=True for a resolved live account). No new bypass.
  • Credential exposure tightened — wire details.caused_by now carries only the exception class name, not str(exc); ctx_metadata fail-close untouched.

Follow-ups (non-blocking — file as issues)

  • brand.json stale-on-error window is dead in the default config. brand_jwks.py:can_serve_stale clamps stale_deadline = min(expires_at + max_stale, fetched_at + DEFAULT_MAX_AGE_SECONDS); with DEFAULT_MAX_AGE_SECONDS=900 and the default no-Cache-Control path where expires_at = fetched_at + 900, the second term collapses the deadline back to expires_at → zero grace. The code comment ("no configuration extends trust past 30 minutes") contradicts the 15-minute value it enforces. Fails closed, so not a block, but the 900+900 split the PR advertises never fires — the ceiling constant should be 1800 (or DEFAULT_MAX_AGE_SECONDS + DEFAULT_MAX_STALE_SECONDS). ad-tech-protocol-expert. Cheap to fix while you're already in the async-JWKS change.
  • Shared default replay store caps all counterparties together. agent_resolver._DEFAULT_REPLAY_STORE is one 1M-entry InMemoryReplayStore partitioned only by keyid namespace, so a single valid signer can exhaust the shared global_cap and fail-closed-reject other counterparties' legitimate requests. Net-new protection (old default was replay_store=None), so it's an improvement — but decouple global_cap from per_keyid_cap or expose it for sizing. security-reviewer: Low.
  • _execute_locked detached task isolates the handler's contextvars. Running the idempotent handler in asyncio.create_task means ContextVars it mutates are no longer visible after the wrapper returns (previously inline). Worth a one-line note in the PR body / changelog. code-reviewer.

Minor nits (non-blocking)

  1. platform_router._run_sync_delegate execution-is-None branch. worker = asyncio.create_task(asyncio.to_thread(...)) then await asyncio.shield(worker) — under caller cancellation the only strong ref is dropped and the task can be GC'd ("Task was destroyed but it is pending"). Not reachable on the deadline-managed get_products path (which always binds an execution), hence a nit.
  2. backends.py per-instance dynamic ContextVar. ContextVar(f"adcp_idempotency_connection_{id(self)}", ...) is the documented dynamic-ContextVar anti-pattern; harmless for a long-lived backend, but prefer one module-level var keyed by backend identity.

Fix the async JWKS resolver and I'll approve. security-reviewer clean, ad-tech-protocol-expert sound-with-caveats, code-reviewer one Major.

@bokelley

Copy link
Copy Markdown
Contributor Author

Addressed Argus’s blocking finding in dd9634e:

  • moved the async expired-cache cooldown decision inside the single-flight lock, re-reading state after waiters acquire it
  • added a concurrent-expiry regression proving waiters block and observe the refreshed JWKS instead of failing
  • also fixed the noted brand.json stale ceiling so the default 15-minute stale-on-error budget is usable while total trust stays capped at 30 minutes

Verification: make pre-push passed (6,381 passed, 41 skipped, 9 deselected, 1 xfailed; coverage 84.55%).

@aao-ipr-bot

aao-ipr-bot Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

@bokelley

Copy link
Copy Markdown
Contributor Author

Maintainer merge note: the blocking Argus finding was fixed in dd9634e and covered by a concurrent-expiry regression. Full make pre-push and every required GitHub check are green. Two fresh Argus executions completed successfully but failed to post a replacement review (the action logged 21 permission denials), leaving the now-stale change request as the only branch-protection blocker. Proceeding with the admin merge path because the review automation, not the code, is blocking.

@bokelley
bokelley disabled auto-merge August 16, 2026 16:43
@bokelley
bokelley merged commit 6fca33c into main Aug 16, 2026
28 checks passed
@bokelley
bokelley deleted the ship-version-7-release branch August 16, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment